Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Major refactor to allow lazy sync iterators. #10

Merged
merged 7 commits into from
Oct 12, 2023

Conversation

aebrahim
Copy link
Member

@aebrahim aebrahim commented Oct 8, 2023

Fixes #6.

Because the code was getting too unweildy and hard to follow, I refactored the iterator proxies to their own classes in a separate file.

Fixes DelfinaCare#6.

Because the code was getting too unweildy and hard to follow, I
refactored the iterator proxies to their own classes in a separate file.
@aebrahim aebrahim requested review from mattalbr and removed request for mattalbr October 8, 2023 05:53
We yield only outside the lock to avoid potential deadlocks.
Copy link
Collaborator

@mattalbr mattalbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More to review but this is the important stuff so far

once/_iterator_wrappers.py Outdated Show resolved Hide resolved
once/_iterator_wrappers.py Outdated Show resolved Hide resolved
once/_iterator_wrappers.py Outdated Show resolved Hide resolved
We use an enum to thoroughly document the possible states the yielding
function can be in, making the code a lot more readable IMHO!
Copy link
Collaborator

@mattalbr mattalbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will read through the tests in detail on the next pass

once/_iterator_wrappers.py Outdated Show resolved Hide resolved
once/_iterator_wrappers.py Outdated Show resolved Hide resolved
once/_iterator_wrappers.py Show resolved Hide resolved
self.finished = True
self.generator = None # Allow this to be GCed.
self.generating = False
else:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It's hard to tell if this else is on the if or the try. I think you could just in-line the else below result = self.generator.send(next_send), like:

try:
    result = self.generator.send(next_send)
    with self.lock:
        self.results.append(result)
        self.generating = False
except StopIteration:
    ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this any clearer?

Copy link
Collaborator

@mattalbr mattalbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Last round I think

once/_iterator_wrappers.py Show resolved Hide resolved
once/_iterator_wrappers.py Outdated Show resolved Hide resolved
once_test.py Show resolved Hide resolved
once_test.py Show resolved Hide resolved
once_test.py Show resolved Hide resolved
Copy link
Collaborator

@mattalbr mattalbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woo we made it! Nice work!

once_test.py Show resolved Hide resolved
@aebrahim aebrahim merged commit 3168628 into DelfinaCare:main Oct 12, 2023
45 checks passed
@aebrahim aebrahim deleted the sync_iterators_lazy branch October 12, 2023 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimization: Evaluate generators lazily
2 participants